Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/config/config.go (1)

51-76: Address redundant validation and zero-override issue.

Two problems:

1. Redundant validation (lines 66-74): After setting defaults when values are zero (lines 54-65), the validation checks on lines 66-74 will always pass. This validation is dead code.

2. Zero-override issue (unresolved from past review): The function overwrites zero values with mainnet defaults, preventing legitimate configurations for Byron-less networks that require EndSlot=0 and Epochs=0. This causes EpochFromSlot to incorrectly compute epochs on such networks.

As suggested in the past review, change EndSlot and Epochs to pointer types (*uint64) in ByronGenesisConfig so you can distinguish "unset" (nil) from explicit zero:

 type ByronGenesisConfig struct {
 	EpochLength        uint64  `yaml:"epoch_length"`
 	ByronSlotsPerEpoch uint64  `yaml:"byron_slots_per_epoch"`
-	EndSlot            uint64  `yaml:"end_slot"`
-	Epochs             uint64  `yaml:"epochs"`
+	EndSlot            *uint64 `yaml:"end_slot"`
+	Epochs             *uint64 `yaml:"epochs"`
 }

Then update populateByronGenesis to apply defaults only when nil:

 func (c *Config) populateByronGenesis() error {
 	cfg := &c.ByronGenesis
 	if cfg.EpochLength == 0 {
 		cfg.EpochLength = 21600
 	}
 	if cfg.ByronSlotsPerEpoch == 0 {
 		cfg.ByronSlotsPerEpoch = cfg.EpochLength
 	}
-	if cfg.EndSlot == 0 {
-		cfg.EndSlot = 4492799
+	if cfg.EndSlot == nil {
+		defaultEndSlot := uint64(4492799)
+		cfg.EndSlot = &defaultEndSlot
 	}
-	if cfg.Epochs == 0 {
-		cfg.Epochs = 208
+	if cfg.Epochs == nil {
+		defaultEpochs := uint64(208)
+		cfg.Epochs = &defaultEpochs
 	}
 	if cfg.EpochLength == 0 {
 		return errors.New("ByronGenesisConfig: EpochLength must be nonzero")
 	}
-	if cfg.EndSlot == 0 {
+	if cfg.EndSlot == nil || *cfg.EndSlot == 0 {
 		return errors.New("ByronGenesisConfig: EndSlot must be nonzero")
 	}
-	if cfg.Epochs == 0 {
+	if cfg.Epochs == nil || *cfg.Epochs == 0 {
 		return errors.New("ByronGenesisConfig: Epochs must be nonzero")
 	}
 	return nil
 }

Update globalConfig initialization and EpochFromSlot to use pointer dereference where needed.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

input/chainsync/chainsync_test.go (1)

81-81: Epoch assertion and comment are correct.

Good fix: Byron-era math is accurate and the EpochNumber=0 assertion matches 12345/21600. BlockNumber=0 on rollback is also clear.

Optionally, add a boundary check to avoid future off‑by‑one regressions:

t.Run("epoch boundary at Byron slot 21600", func(t *testing.T) {
	c := &ChainSync{status: &ChainSyncStatus{}, eventChan: make(chan event.Event, 1)}
	point := ocommon.Point{Slot: 21600}
	tip := chainsync.Tip{Point: ocommon.Point{Slot: 21600}}
	require.NoError(t, c.handleRollBackward(chainsync.CallbackContext{}, point, tip))
	assert.Equal(t, uint64(21600), c.status.SlotNumber)
	assert.Equal(t, uint64(1), c.status.EpochNumber) // slots 0..21599 => epoch 0; slot 21600 => epoch 1
})

Also applies to: 85-86

internal/config/config.go (4)

28-34: Good: pointers enable explicit zero for Byron-less networks. Consider env var tags for nested fields.

Switching EndSlot/Epochs to pointers unblocks zero-valued configs. To make env variables ergonomic/consistent for these nested fields, add explicit env tags (and/or split_words) so operators can set them via env without surprising names.

 type ByronGenesisConfig struct {
-    EpochLength        uint64  `yaml:"epoch_length"`
-    ByronSlotsPerEpoch uint64  `yaml:"byron_slots_per_epoch"`
-    EndSlot            *uint64 `yaml:"end_slot"`
-    Epochs             *uint64 `yaml:"epochs"`
+    EpochLength        uint64  `yaml:"epoch_length"        envconfig:"EPOCH_LENGTH"        split_words:"true"`
+    ByronSlotsPerEpoch uint64  `yaml:"byron_slots_per_epoch" envconfig:"SLOTS_PER_EPOCH"   split_words:"true"`
+    EndSlot            *uint64 `yaml:"end_slot"            envconfig:"END_SLOT"`
+    Epochs             *uint64 `yaml:"epochs"              envconfig:"EPOCHS"`
 }

And for Shelley:

 type ShelleyGenesisConfig struct {
-    EpochLength uint64 `yaml:"epoch_length"`
+    EpochLength uint64 `yaml:"epoch_length" envconfig:"EPOCH_LENGTH" split_words:"true"`
 }

---

47-74: Guard against divergence between EpochLength and ByronSlotsPerEpoch.

If both are provided and differ, behavior becomes ambiguous. Add a fast-fail (or normalize) to keep them consistent.

 func (c *Config) populateByronGenesis() error {
     cfg := &c.ByronGenesis
     if cfg.EpochLength == 0 {
         cfg.EpochLength = 21600
     }
-    if cfg.ByronSlotsPerEpoch == 0 {
-        cfg.ByronSlotsPerEpoch = cfg.EpochLength
-    }
+    if cfg.ByronSlotsPerEpoch == 0 {
+        cfg.ByronSlotsPerEpoch = cfg.EpochLength
+    } else if cfg.EpochLength != 0 && cfg.ByronSlotsPerEpoch != cfg.EpochLength {
+        return errors.New("ByronGenesisConfig: ByronSlotsPerEpoch must equal EpochLength")
+    }
     if cfg.EndSlot == nil {
         defaultEndSlot := uint64(4492799)
         cfg.EndSlot = &defaultEndSlot
     }

If you intend to deprecate one of these fields, consider marking one as canonical and documenting precedence. Based on learnings.

---

133-141: Set ByronSlotsPerEpoch explicitly in defaults to avoid pre-populate reads.

Some code/tests may access globalConfig before Load()/populate*. Being explicit prevents nil/zero surprises.

 ByronGenesis: ByronGenesisConfig{
     EpochLength: 21600,
+    ByronSlotsPerEpoch: 21600,
     EndSlot:     func() *uint64 { v := uint64(4492799); return &v }(),
     Epochs:      func() *uint64 { v := uint64(208); return &v }(),
 },

---

89-101: Tag nested struct fields for envconfig
Nested ByronGenesisConfig and ShelleyGenesisConfig fields (e.g. EpochLength, ByronSlotsPerEpoch) default to DUMMY_BYRONGENESIS_EPOCHLENGTH (no underscores) when processed by envconfig.Process("DUMMY", &c) (github.com). To generate DUMMY_BYRON_GENESIS_EPOCH_LENGTH (and similarly for other nested fields), add split_words:"true" or explicit envconfig:"BYRON_GENESIS_EPOCH_LENGTH" tags to each field.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

input/chainsync/chainsync.go (1)

43-69: Division-by-zero guards return potentially incorrect values.

The guards at lines 58-59 and 65-66 prevent panics but return hardcoded fallback values when epoch lengths are zero. As noted in past reviews, this can silently produce incorrect epoch numbers if the genesis configuration is invalid or missing validations.

Since past reviews have already flagged this issue and suggested returning an error instead of a best-guess value, please address that feedback by either:

1. Ensuring validation in populateByronGenesis() and populateShelleyGenesis() makes zero epoch lengths impossible, or
2. Returning an error from this function when invalid configuration is encountered

filter/chainsync/chainsync_test.go (2)

95-98: Cosmetic field reordering in MockOutput.

The field order in MockOutput was changed (scriptRef moved before amount), but this is purely organizational and doesn't affect functionality. Go struct field order doesn't impact behavior, only memory layout.

---

209-219: Cosmetic field reordering in mock certificates.

The embedded struct fields were moved after cborData in both mockStakeDelegationCert and mockStakeDeregistrationCert. This is a cosmetic change that doesn't affect functionality.

input/chainsync/block_test.go (2)

24-33: Cosmetic field reordering in MockBlockHeader.

The fields in MockBlockHeader were reordered (era and cborBytes moved earlier), but this is purely organizational. The accessor methods remain unchanged, so test functionality is unaffected.

---

114-121: Field order changes in test cases are cosmetic.

The test case struct fields were reordered, but this doesn't affect test behavior. All fields are still properly initialized and assertions remain correct.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

input/chainsync/chainsync.go (1)

42-74: Consider error handling for invalid genesis configuration

The EpochFromSlot function returns fallback values when epoch lengths are zero (lines 63-64, 70-71), which may produce incorrect epoch numbers if the genesis configuration is invalid. A previous review suggested returning an error instead of a best-guess value.

However, the current implementation with fallback values is pragmatic if:

1. The config population methods (populateByronGenesis() and populateShelleyGenesis()) always ensure non-zero epoch lengths
2. Zero-length epochs are an acceptable edge case for testing or non-standard networks

Recommendation: Document the assumptions in the function comment that epoch lengths must be non-zero in normal operation, and that the fallback values are defensive programming for edge cases.

Add documentation:

-// EpochFromSlot derives an epoch from a slot using Byron/Shelley genesis params.
-// Byron slots: 0..EndSlot inclusive. Explicit zero EndSlot/Epochs means no Byron era.
-// Zero epoch length in either era yields a safe fallback (0 Byron / starting Shelley epoch).
+// EpochFromSlot derives an epoch from a slot using Byron/Shelley genesis params.
+// Byron slots: 0..EndSlot inclusive. Explicit zero EndSlot/Epochs means no Byron era.
+// Zero epoch length in either era yields a safe fallback (0 Byron / starting Shelley epoch).
+// 
+// Assumptions:
+// - populateByronGenesis() and populateShelleyGenesis() ensure non-zero epoch lengths in normal configs
+// - Zero-length fallbacks are defensive programming for edge cases and testing
+// - For production use, genesis config should always have valid epoch lengths
 func EpochFromSlot(slot uint64) uint64 {

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

input/chainsync/chainsync.go (1)

42-75: Document zero-epoch-length behavior or consider validation.

The division-by-zero guards (lines 64-66, 71-73) return fallback values that may be incorrect:

• Byron era: returns 0 for all slots when EpochLength == 0, even for slots > 0
• Shelley era: returns shelleyStartEpoch for all slots when EpochLength == 0, regardless of actual slot distance

While the config defaults ensure non-zero epoch lengths, explicit zero configurations (e.g., misconfigured Byron-less networks) would silently produce incorrect epoch numbers.

Recommendation: Add a brief doc comment explaining that this function assumes epoch lengths are non-zero (ensured by config population) and that zero-length configurations will return fallback values rather than errors.

Consider adding to the function comment:

// EpochFromSlot derives an epoch from a slot using Byron/Shelley genesis params.
// Assumes epoch lengths are non-zero (enforced by config defaults).
// Byron slots: 0..EndSlot inclusive. Explicit zero EndSlot/ShelleyTransEpoch means no Byron era.
// Zero epoch length in either era yields a safe fallback (0 Byron / starting Shelley epoch).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro